deploy: short-circuit pull if digest pullspec already exists #1380
Conversation
|
If we do this, I'll probably add the same to rpm-ostree (... will be nice once we get to coreos/rpm-ostree#4994). Also this needs a test. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
cgwalters
force-pushed
the
pr/digest-pull
branch
from
efe5793 to
1ef2d57
Compare
|
WFM. Just to note though, this was an attempt to match the "No changes" code lower down for the
Heh, that was actually what I did at first, trying to make it directly part of Or do you mean just shoving that code into a non-member utility function in ostree-ext? |
|
Alternatively I guess we switch |
|
Yep let me take this one |
Prep for avoiding any operations in pull-by-digest mode. Signed-off-by: Colin Walters <[email protected]>
Prep for pull-by-digest noop. Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
pr/digest-pull
branch
from
1ef2d57 to
f49ed5a
Compare
|
OK I redid this, can you review? |
If we're pulling by digest and the pullspec already exists, then there's no need to reach out to the registry or even spawn skopeo. Detect this case and exit early in the pull code. This allows RHCOS to conform better to the PinnedImageSet API[1], where the expectation is that once an image is pulled, the registry will not be contacted again. In a future with unified storage, the MCO's pre-pull would work just the same for the RHCOS image as any other. Framing this more generally: this patch allows one to pre-pull an image into the store, and making the later deployment operation be fully offline. E.g. this could be used to implement a `bootc switch --download-only` option. [1] https://github.com/openshift/enhancements/blob/26ce3cd8a0c7ce650e73bc5393a3605022cb6847/enhancements/machine-config/pin-and-pre-load-images.md Signed-off-by: Colin Walters <[email protected]> Co-authored-by: Colin Walters <[email protected]> Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
pr/digest-pull
branch
from
f49ed5a to
8da5557
Compare
| } | ||
| Some(previous_state) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
Shouldn't this be previous_state?
There was a problem hiding this comment.
No, because we destructured above - here we know previous_state is None.
|
|
||
| // Parse the target reference to see if it's a digested pull | ||
| let target_reference = self.imgref.imgref.name.parse::<Reference>().ok(); | ||
| let previous_state = if let Some(target_digest) = target_reference |
There was a problem hiding this comment.
Yeah... I know why we do this (to not lose ownership of previous_state). What I had done in my first attempt which was in this same spot was:
let oci_ref = self.imgref.imgref.name.parse::<OciReference>().ok();
if let Some(digest) = oci_ref.as_ref().and_then(|oci_ref| oci_ref.digest()) {
let digest = Digest::from_str(digest)?;
if previous_state.as_ref()
.map(|state| state.manifest_digest == digest)
.unwrap_or_default()
{
// SAFETY: It can't be None if the map().unwrap() above gave
// true. We do it this way so that we only consume the Option if
// we return here. That way it can be reused below if not.
return Ok(PrepareResult::AlreadyPresent(previous_state.unwrap()));
}
}which yes, does an unwrap (though is guaranteed safe), but has a cleaner flow.
There was a problem hiding this comment.
That version is definitely a lot cleaner to read, but I usually try pretty hard to avoid unwrap().
|
I can't officially LGTM, but LGTM. Thanks for picking this up! Having it in ostree-ext means rpm-ostree should also get this behaviour. |
3 participants
If we're pulling by digest and the pullspec already exists, then there's no need to reach out to the registry or even spawn skopeo.
Detect this case and exit early in the pull code.
This allows RHCOS to conform better to the PinnedImageSet API[1], where the expectation is that once an image is pulled, the registry will not be contacted again. In a future with unified storage, the MCO's pre-pull would work just the same for the RHCOS image as any other.
Framing this more generally: this patch allows one to pre-pull an image into the store, and making the later deployment operation be fully offline. E.g. this could be used to implement a
bootc switch --download-onlyoption.[1] https://github.com/openshift/enhancements/blob/26ce3cd8a0c7ce650e73bc5393a3605022cb6847/enhancements/machine-config/pin-and-pre-load-images.md